Port takeover-dialog files to TypeScript #2790
Conversation
- Port all 6 files in src/app/layouts/default/takeover-dialog to .tsx - Add type annotations following guidelines: avoid any, prefer type over interface - Use inline type definitions for function parameters - Rely on type inference where possible - Keep line lengths under 120 characters Related to CORE-1265 🤖 Generated with [Claude Code](https://claude.com/claude-code) Make children prop optional to fix TypeScript type issue The project has incomplete React type definitions, causing TypeScript to not recognize children passed via JSX syntax. Making the prop optional resolves this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript array concat issue in takeover-dialog Replace array concat pattern with explicit array handling to satisfy TypeScript type checking. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
RoyEJohnson
force-pushed
the
port-takeover-dialog-ts
branch
from
1706634 to
9364a28
Compare
There was a problem hiding this comment.
See the first commit for the real changes for all the files in the takeover-dialog folder. It's all just adding types. The final commit is prettier, which is what changed the file enough that it registered as delete-and-create.
There was a problem hiding this comment.
the overlapping-quote section got removed in the last assignable page update but the supporting source files weren't cleaned up.
| import booksForAnalyticsData from '../src/data/books-for-analytics'; | ||
| import bookTitleData from '../src/data/book-titles'; | ||
| import buyprintData from '../src/data/buyprint'; | ||
| import chemistryData from '../src/data/chemistry-atoms-first.json'; |
There was a problem hiding this comment.
Added a book with a different book_state to test non-live book display
There was a problem hiding this comment.
Extracted from default.test for re-use.
There was a problem hiding this comment.
There were a few files that lacked test coverage for little bits. This is one.
jivey
left a comment
jivey
left a comment
There was a problem hiding this comment.
A little concerned about the non-null assertion but everything else seems good
| <MessageBox | ||
| buttonText={data.buttonText} | ||
| buttonUrl={data.buttonUrl} | ||
| headline={data.boxHeadline!} |
There was a problem hiding this comment.
You could add undefined to the MessageBox prop types instead using assertions, seems like it would catch issues with future changes to that component.
There was a problem hiding this comment.
Made the fields in the data that comes from the CMS non-optional. It includes them even if they're empty. I might have made them optional when they were first added so they'd be backward compatible with the CMS until it updated.
Summary
Port all 6 JavaScript files in
src/app/layouts/default/takeover-dialogto TypeScript following the project's guidelines.Changes
takeover-context.js→takeover-context.tsxcommon.js→common.tsxcontent-desktop.js→content-desktop.tsxcontent-mobile.js→content-mobile.tsxtakeover-dialog-content.js→takeover-dialog-content.tsxtakeover-dialog.js→takeover-dialog.tsxGuidelines Followed
anytypetypeoverinterfaceTest Plan
Related to: https://openstax.atlassian.net/browse/CORE-1265
🤖 Generated with Claude Code